SNP Integration Test: Improve Clarity #1621
Conversation
phip1611
force-pushed
the
snp-test
branch
2 times, most recently
from
ca3266b to
e24e342
Compare
phip1611
changed the title
phip1611
force-pushed
the
snp-test
branch
2 times, most recently
from
6c758f5 to
ff20426
Compare
|
FYI: I wouldn't count on interrupt status returning anything useful. I'm actually somewhat surprised to find this in the SNP spec (without much details what the expected behavior is). UEFI does not use interrupts (except timer) and handles devices in polling mode. So there is little reason for uefi drivers to configure device interrupt registers in the first place. |
phip1611
force-pushed
the
snp-test
branch
5 times, most recently
from
ad4b06f to
2c881ba
Compare
uefi/src/proto/network/snp.rs
Outdated
| @@ -186,13 +186,36 @@ impl SimpleNetwork { | |||
| status.to_result_with_val(|| NonNull::new(tx_buf.cast())) | |||
| } | |||
|
|
|||
| /// Place a packet in the transmit queue of a network interface. | |||
| /// Place a packet in the transmit queue of the network interface. | |||
There was a problem hiding this comment.
Could the changes in this file be moved to a separate PR? I'd like to review them in isolation from the test changes.
There was a problem hiding this comment.
For the record: opened #1664
This will help future debugging of this test and the mental model of what's happening. Interestingly, the interrupt status never shows that we can receive a packet. Probably not implemented by OVMF?
This helps to identify the network interface in integration tests. So far, we also only use a single network device. In case there are more, the next device should have a similar MAC address, such as the last component being incremented by one.
| @@ -4,10 +4,18 @@ pub fn test() { | |||
| info!("Testing Network protocols"); | |||
|
|
|||
| http::test(); | |||
| pxe::test(); | |||
| snp::test(); | |||
| #[cfg(feature = "pxe")] | |||
There was a problem hiding this comment.
Let's change this to disabling at runtime like how pxe.rs did it before this PR -- if cfg!(feature = "pxe") { ... } . That way, we always get a compilation test, even when we're not running the code.
| res.inspect(|_| { | ||
| debug!("Received:"); | ||
| debug!(" src_mac = {:x?}", recv_src_mac); | ||
| debug!(" dst_mac = {:x?}", recv_dst_mac); |
There was a problem hiding this comment.
This is now a fixed value right? So let's assert that it matches 52:54:00:00:00:01 rather than printing it.
| debug!("Received:"); | ||
| debug!(" src_mac = {:x?}", recv_src_mac); | ||
| debug!(" dst_mac = {:x?}", recv_dst_mac); | ||
| debug!(" ethernet_proto=0x{:x?}", recv_ethernet_protocol); |
There was a problem hiding this comment.
This is asserted below so no need to print.
(In general, I would like to stick to asserting rather than printing as much as possible in tests, because 99% of the time we're not going to look that carefully at the output and won't notice if something doesn't match expectations. For ad-hoc debugging it's easy enough to throw a log line in while working on the code.)
| if simple_network.receive(&mut buffer, None, None, None, None) | ||
| == Err(Status::NOT_READY.into()) | ||
| { | ||
| boot::stall(Duration::from_secs(1)); |
There was a problem hiding this comment.
Are we sure this stall is no longer needed? I'm guessing it was added because sometimes the interface isn't ready yet, although I'm not clear on the details from a quick look at the commit history.
| continue; | ||
| }; | ||
| // Print device path |
There was a problem hiding this comment.
I'm not clear on why it's useful to see the device path for the network interface.
| simple_network | ||
| .start() | ||
| .expect("Failed to start Simple Network"); | ||
| .expect("Network should not be started yet"); |
There was a problem hiding this comment.
start can fail for other reasons, so I think the previous text was more accurate
| simple_network | ||
| .initialize(0, 0) | ||
| .expect("Failed to initialize Simple Network"); | ||
| .expect("Network should not be initialized yet"); |
There was a problem hiding this comment.
Same here, initialize can fail for other reasons.
TL;DR: This adds more clarity to facilitate future debugging and improvements for #1617.
Wow, this was an unexpected major time sink. The problem here is that if
snp::testruns beforepxe::test,snp.receive()receives an Ethernet ARP package rather than a Ethernet IPv4 packet.I tried two solutions:
I got non working, unfortunately. So instead, this adds more clarity to ease future debugging and improvements.
Relates to #1617 opened by @kraxel.
Steps to Undraft
Checklist